Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Prometheus-readable metrics endpoint #194

Merged
merged 10 commits into from
Jan 3, 2023
Merged

Add Prometheus-readable metrics endpoint #194

merged 10 commits into from
Jan 3, 2023

Conversation

diegodiv
Copy link

@diegodiv diegodiv commented Jan 2, 2023

Fixes part of #99 by showing Prometheus-readable metrics on a configurable port, only showing one metric (number of PRs merged) at the moment. Other metrics will be easier to add now, but they should be described in new issues, in more specific detail, when the need arises.

@diegodiv diegodiv requested review from bertptrs and FPtje January 2, 2023 13:34
@diegodiv diegodiv self-assigned this Jan 2, 2023
@diegodiv diegodiv changed the title Metrics Add Prometheus-readable metrics endpoint Jan 2, 2023
Copy link

@bertptrs bertptrs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't read Haskell very well so I can't comment too much, but I have one suggestion

Right now we have a single counter for the number of PRs merged. This is not that helpful if you have many projects in a single instance like we do. Perhaps we could add a project label to the counter and track the metric by project?

@diegodiv
Copy link
Author

diegodiv commented Jan 2, 2023

I don't read Haskell very well so I can't comment too much, but I have one suggestion

Right now we have a single counter for the number of PRs merged. This is not that helpful if you have many projects in a single instance like we do. Perhaps we could add a project label to the counter and track the metric by project?

Sorry for the lack of clarity, that's what it already does!

Copy link

@FPtje FPtje left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! This is going to give some wonderful insights!

Some comments, and a general note:

Could you test this PR and make sure:

  • The counter is indeed increased when a PR is merged
  • The /metrics endpoint gives a good output (you could post it in this PR)

nix/haskell-dependencies.nix Outdated Show resolved Hide resolved
src/Logic.hs Outdated Show resolved Hide resolved
src/Logic.hs Outdated Show resolved Hide resolved
src/EventLoop.hs Outdated Show resolved Hide resolved
app/Main.hs Show resolved Hide resolved
app/Main.hs Outdated Show resolved Hide resolved
app/Main.hs Outdated Show resolved Hide resolved
app/Main.hs Outdated Show resolved Hide resolved
app/Main.hs Show resolved Hide resolved
app/Main.hs Outdated Show resolved Hide resolved
@diegodiv diegodiv requested a review from FPtje January 3, 2023 10:33
Copy link

@FPtje FPtje left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, one nit but otherwise LGTM!

src/Configuration.hs Outdated Show resolved Hide resolved
src/Metrics/Metrics.hs Outdated Show resolved Hide resolved
@diegodiv diegodiv requested a review from FPtje January 3, 2023 15:28
Copy link

@FPtje FPtje left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

@diegodiv
Copy link
Author

diegodiv commented Jan 3, 2023

@OpsBotPrime merge

@OpsBotPrime
Copy link

Pull request approved for merge by @DiegoDiverio, rebasing now.

Approved-by: DiegoDiverio
Auto-deploy: false
@OpsBotPrime
Copy link

Rebased as 7e9026c, waiting for CI …

@OpsBotPrime
Copy link

CI job 🟡 started.

@OpsBotPrime OpsBotPrime merged commit 7e9026c into master Jan 3, 2023
@OpsBotPrime OpsBotPrime deleted the metrics branch January 3, 2023 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants